-
Notifications
You must be signed in to change notification settings - Fork 140
Implement customizable transition animation duration setting #2321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Implement customizable transition animation duration setting #2321
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #2321 +/- ##
==========================================
+ Coverage 68.85% 69.39% +0.54%
==========================================
Files 90 90
Lines 7612 7753 +141
==========================================
+ Hits 5241 5380 +139
- Misses 2371 2373 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Comment from @sarthak-19 in Slack:
|
| ?> | ||
| <style> | ||
| @view-transition { navigation: auto; } | ||
| ::view-transition-group(*) { animation-duration: <?php echo esc_html( (string) $duration_seconds ); ?>s; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esc_attr is probably more appropriate here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, neither is appropriate, as I've come to find. The contents of STYLE are the same as SCRIPT in that they are “raw text”. Entities are not processed.
In any case, since the value is an integer anyway, we can ensure proper escaping via:
| ::view-transition-group(*) { animation-duration: <?php echo esc_html( (string) $duration_seconds ); ?>s; } | |
| ::view-transition-group(*) { animation-duration: <?php echo (int) $duration_seconds; ?>s; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that is best. esc_html felt too broad, the int cast is perfect.
| ?> | ||
| <input | ||
| type="number" | ||
| id="<?php echo $field_id; ?>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| id="<?php echo $field_id; ?>" | |
| id="<?php echo esc_attr( $args['label_for'] ); ?>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use esc_attr( $args['label_for'] ) multiple times that's why I used $field_id variable like :
$field_id = esc_attr( $args['label_for'] );Is there something wrong with this approach ?
Since I'm using field_id like :
<span class="description">
<?php
printf(
/* translators: %s: duration in seconds wrapped in a span for dynamic update */
esc_html__( 'ms (%s)', 'view-transitions' ),
'<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>'
);
?>
</span>
<?php
wp_print_inline_script_tag(
sprintf(
'document.getElementById( %s ).addEventListener( "input", function( e ) { document.getElementById( %s ).textContent = ( parseInt( e.target.value, 10 ) / 1000 ) + "s"; } );',
wp_json_encode( $field_id ),
wp_json_encode( $field_id . '-seconds' )
)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, better to not do that. It's better to always use an escaping function wherever a variable is being printed. While there would be no vulnerability the way you have it now, at some point in the future something could change to that variable to make it vulnerable. Also, every time a developer looks at the printing they'd have to go up to make sure it had been escaped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh okay, make sense, I'll keep this in mind moving forward, I was just trying to follow DRY design principle!!
| printf( | ||
| /* translators: %s: duration in seconds wrapped in a span for dynamic update */ | ||
| esc_html__( 'ms (%s)', 'view-transitions' ), | ||
| '<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| '<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>' | |
| '<span id="' . esc_attr( $args['label_for'] . '-seconds' ) . '">' . esc_html( (string) $seconds ) . 's</span>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, same doubt : #2321 (comment)
| /* translators: %s: duration in seconds wrapped in a span for dynamic update */ | ||
| esc_html__( 'ms (%s)', 'view-transitions' ), | ||
| '<span id="' . $field_id . '-seconds">' . esc_html( (string) $seconds ) . 's</span>' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need the seconds? Can't we just have the milliseconds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have, but as a part of polishing feature it can be a simple yet effective thing from end user perspective don't you think ?
I mean I'm open to discussion here, can remove if it's overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be removed. The UI here is not going to be proposed for core, so it's for power users of the plugin. They already have to be comfortable with CSS selectors, so they should be comfortable with milliseconds as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure then I'll remove this and push the updated changes.
| wp_print_inline_script_tag( | ||
| sprintf( | ||
| 'document.getElementById( %s ).addEventListener( "input", function( e ) { document.getElementById( %s ).textContent = ( parseInt( e.target.value, 10 ) / 1000 ) + "s"; } );', | ||
| wp_json_encode( $field_id ), | ||
| wp_json_encode( $field_id . '-seconds' ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, I don't think we need this.
| wp_print_inline_script_tag( | |
| sprintf( | |
| 'document.getElementById( %s ).addEventListener( "input", function( e ) { document.getElementById( %s ).textContent = ( parseInt( e.target.value, 10 ) / 1000 ) + "s"; } );', | |
| wp_json_encode( $field_id ), | |
| wp_json_encode( $field_id . '-seconds' ) | |
| ) | |
| ); |
| } elseif ( 'number' === $type ) { | ||
| $seconds = (int) $value / 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a general field output for number but it presumes the number value is going to be seconds or time. That doesn't seem ideal. Note how the checkbox condition above is agnostic to what the setting is for.
| min="<?php echo esc_attr( (string) PLVT_MIN_ANIMATION_DURATION ); ?>" | ||
| max="<?php echo esc_attr( (string) PLVT_MAX_ANIMATION_DURATION ); ?>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should come from the $args
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @sarthak.jaiswal@rtCamp.com. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| exit; // Exit if accessed directly. | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was marked as resolved.
This comment was marked as resolved.
|
Another polishing idea: there could be validation added to the input fields for the CSS selectors. Maybe this would be as simple as a |
Part of a polishing issue : #2317
Summary
This PR implements the transition animation duration setting that was previously added to the UI but wasn't functional. The duration setting now properly affects view transitions on both the frontend and WordPress admin area.